Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Matlab legacy fix #1414

Merged
merged 1 commit into from
Dec 15, 2022
Merged

Matlab legacy fix #1414

merged 1 commit into from
Dec 15, 2022

Conversation

ssun30
Copy link
Contributor

@ssun30 ssun30 commented Dec 10, 2022

Changes proposed in this pull request

  • Fix several Matlab (MEX interface) samples due to changes in several YAML files.
  • Added missing blank lines at the end of three files.
  • Reformat files to match Cantera contribution guides.

If applicable, fill in the issue number this pull request is fixing

  • In current release of Cantera, (2.6.0) some Matlab examples would return an error due to changes in phase names in several YAML files.
  • An example is h2o2.yaml, where the 'gas' phase is renamed to 'ohmech'. This affects examples that use this specific YAML file.
  • With this update, the auto test script test_examples.m will run all examples without issues.

Checklist

  • The pull request includes a clear description of this code change
  • Commit messages have short titles and reference relevant issues
  • Build passes (scons build & scons test) and unit tests address code coverage
  • Style & formatting of contributed code follows contributing guidelines
  • The pull request is ready for review

ischoegl
ischoegl reviewed Dec 10, 2022
View reviewed changes
Copy link
Member

@ischoegl ischoegl left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ssun30 ... thanks for the bug fixes - the incorrect phase for the Solution was something that was introduced a while ago (#1109, I didn't catch this in my review) - in an earlier version, this used GRI-Mech, where the phase was correct.

I'm not sure that it's worth changing sample names or update formatting (beyond the newlines) - as this will become legacy code soon, I'd rather leave things as is, even if this doesn't exactly fit the mold.

As the remaining changes are modest, please squash your commits.

PS: Please feel free to add yourself to the AUTHORS file in the root folder

samples/matlab/diff_flame.m Outdated Show resolved Hide resolved
samples/matlab/plug_flow_reactor.m Outdated Show resolved Hide resolved
samples/matlab/test_examples.m Outdated Show resolved Hide resolved
samples/matlab/ignite_hp.m Show resolved Hide resolved
samples/matlab/test_examples.m Show resolved Hide resolved
@ssun30 ssun30 marked this pull request as ready for review December 15, 2022 03:22
@ssun30
Copy link
Contributor Author

ssun30 commented Dec 15, 2022

I've squashed all the changes and addressed all reviews. I think this is ready for merge.

@codecov
Copy link

codecov bot commented Dec 15, 2022

Codecov Report

Merging #1414 (aff6c97) into main (6042429) will decrease coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #1414      +/-   ##
==========================================
- Coverage   70.79%   70.78%   -0.02%     
==========================================
  Files         373      373              
  Lines       53872    53872              
  Branches    17579    17579              
==========================================
- Hits        38139    38131       -8     
- Misses      13332    13339       +7     
- Partials     2401     2402       +1
Impacted Files Coverage Δ
...terfaces/dotnet/Cantera/src/Interop/InteropUtil.cs 60.75% <0.00%> (-8.87%) ⬇️
src/base/stringUtils.cpp 79.03% <0.00%> (-0.81%) ⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

ischoegl
ischoegl approved these changes Dec 15, 2022
View reviewed changes
Copy link
Member

@ischoegl ischoegl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @ssun30 - this looks good to me!

@ischoegl ischoegl merged commit 2ce92ea into Cantera:main Dec 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ischoegl ischoegl ischoegl approved these changes

Assignees
No one assigned
Labels
bug Matlab
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ssun30 @ischoegl